Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: change error for invalid feature param in feature RPC #5063

Merged
merged 5 commits into from
Jul 30, 2024

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Jul 11, 2024

High Level Overview of Change

This PR changes the error message for invalid values of feature in the feature RPC. Now, if a non-string is passed into the function, it will return invalidParams, instead of badFeature as it did before.

Context of Change

In rippled 2.2.0, a non-admin version of feature was added. This makes the need for proper error handling more important.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

API Impact

  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)

While this is technically a breaking change, I consider it more of a bugfix. Only the error message is changing.

Test Plan

Additional unit tests were added.

@mvadari mvadari requested a review from godexsoft July 11, 2024 14:38
@mvadari mvadari force-pushed the bad-feature branch 2 times, most recently from 11d072c to 846dcf3 Compare July 11, 2024 14:45
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.4%. Comparing base (2820feb) to head (61e6e85).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5063     +/-   ##
=========================================
- Coverage     71.4%   71.4%   -0.0%     
=========================================
  Files          796     796             
  Lines        67039   67042      +3     
  Branches     10864   10864             
=========================================
+ Hits         47844   47846      +2     
- Misses       19195   19196      +1     
Files Coverage Δ
src/xrpld/rpc/handlers/Feature1.cpp 94.3% <100.0%> (+0.5%) ⬆️

... and 5 files with indirect coverage changes

Impacted file tree graph

Copy link
Collaborator

@godexsoft godexsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me 👍

Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK because this is a relatively new RPC, and it is not involved in how integrations do tx processing (where any potential API breakage would be more painful)

@mvadari
Copy link
Collaborator Author

mvadari commented Jul 29, 2024

This is ready to merge.

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 29, 2024
@ximinez ximinez merged commit a39720e into XRPLF:develop Jul 30, 2024
19 of 20 checks passed
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
…#5063)

* Returns an "Invalid parameters" error if the `feature` parameter is provided and is not a string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants